Re-clustering API bug fixes #2397
Conversation
|
@MohamedElgammal some tests failed due to download issues --> relaunch? |
| @@ -157,6 +165,8 @@ bool swap_two_molecules(t_pack_molecule* molecule_1, | |||
| bool during_packing, | |||
| int verbosity, | |||
| t_clustering_data& clustering_data) { | |||
| auto& cluster_ctx = g_vpr_ctx.mutable_clustering(); | |||
There was a problem hiding this comment.
I put this on Amin's MR, but why is this declared so far away from where it is used? Is this a style thing?
| t_pb* clb_pb_1 = cluster_ctx.clb_nlist.block_pb(clb_1); | ||
| std::string clb_pb_1_name = (std::string)clb_pb_1->name; | ||
| t_pb* clb_pb_2 = cluster_ctx.clb_nlist.block_pb(clb_2); | ||
| std::string clb_pb_2_name = (std::string)clb_pb_2->name; |
There was a problem hiding this comment.
I also put this on Amins PR lol, but I think these should use the std::string's constructor instead of using the c-style casting
| const int mode, | ||
| const int feasible_block_array_size, | ||
| const int& mode, | ||
| const int& feasible_block_array_size, |
There was a problem hiding this comment.
Why were these made pass by reference? Did you see any performance increase after changing this?
There was a problem hiding this comment.
Passing an int by constant reference isn't going to be faster and might be slower as a pointer is 64-bits and an int is 32, and pointer chasing is slow. After compiler optimization the whole pointer/reference stuff might go away, but generally it is better to pass things smaller than or equal to pointer size by value; use const & for bigger things that aren't changed in a function.
There was a problem hiding this comment.
So I suggest putting these back to const int, although it isn't a huge deal.
|
@MohamedElgammal : should this PR be landed? |
|
I am also interested in these fixes. |
|
looking good to me .. We need to have a second look on the failing tests ... let me rerun them and update you |
|
I think the 4 Clang CI build failures are caused by this branch being behind VTR master by some time. I think rebasing it onto VTR master should resolve the issue. @MohamedElgammal could you rebase the branch? I see a button to do so, but I am not sure if doing it from the GitHub UI would cause any issues. |
AlexandreSinger
force-pushed
the
reclustering-api-bug-fixes
branch
from
4640730 to
b95d1e3
Compare
|
I have rebased the branch using the GitHub UI. Hopefully the tests should pass now and this can be merged in. |
|
Great, thanks. @MohamedElgammal : any barrier to merging this if CI passes? |
|
@AlexandreSinger : I believe this is the branch that has an example of how to use the re-clustering API. |
|
@AlexandreSinger : Thanks for rebasing it. |
|
Thanks Mohamed. Merging! |
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist: